Add a timer for scan responses#410
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughArr: This PR adds scan-response timeout handling to NimBLEScan (waiting list, timer, stats), integrates SR timeout logic into GAP event processing (enqueue/dequeue, DISC_COMPLETE draining), and initializes the Changes
Sequence Diagram(s)sequenceDiagram
participant GAP as BLE GAP Event
participant Scan as NimBLEScan
participant Wait as WaitingList
participant Timer as SR Timer
participant CB as Scan Callback
GAP->>Scan: DISC result (new adv)
Scan->>Scan: create AdvertisedDevice, set m_time
Scan->>Wait: addWaitingDevice() (if scannable)
Scan->>Timer: resetWaitingTimer() (start head timeout)
GAP->>Scan: DISC result (scan response)
Scan->>Wait: removeWaitingDevice()
Scan->>CB: onResult() with SR
Timer->>Scan: srTimerCb() (timeout fired)
Scan->>Wait: pop head (missed SR)
Scan->>CB: onResult() (mark missed SR / m_callbackSent=2)
GAP->>Scan: DISC_COMPLETE
Scan->>Timer: stopWaitingTimer()
Scan->>Wait: clearWaitingList()
Scan->>CB: onResult() for remaining waiting devices
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NimBLEScan.cpp`:
- Around line 57-64: The timeout branch in NimBLEScan:: (where
removeWaitingDevice(pDev) sets pDev->m_callbackSent = 2 and invokes
m_pScanCallbacks->onResult) never updates the missed-SR metric, so add a metric
update there: call the scan's incMissedSrCount() (or increment a new
timed-out-waiters counter on the scan, e.g., m_timedOutWaiters) immediately when
handling this timeout, and ensure the scan-end logic (where incMissedSrCount()
is currently called for other cases) reconciles that counter by adding its value
to the final missed-SR tally before logging; reference symbols:
removeWaitingDevice, m_callbackSent, m_pScanCallbacks->onResult,
incMissedSrCount (or m_timedOutWaiters) and the scan-end aggregation code that
currently increments missed SRs.
- Around line 354-367: The pending snapshot currently stores raw
NimBLEAdvertisedDevice* and can dangle if clearResults() deletes devices during
onResult() callbacks; change the snapshot to own or hold safe references (e.g.,
copy the NimBLEAdvertisedDevice objects or convert m_scanResults.m_deviceVec
entries to shared_ptr and make pending a
vector<shared_ptr<NimBLEAdvertisedDevice>>) so the pointees remain valid while
the loop calling m_pScanCallbacks->onResult(dev) runs, or alternatively
gate/deschedule destructive operations by setting a flag on the scan (e.g., in
pScan) to defer clearResults() until after the callback loop completes; update
code paths that mutate m_scanResults (clearResults, BLE_GAP_EVENT_DISC_COMPLETE
handling) to respect the chosen protection mechanism and ensure m_callbackSent
logic still applies.
- Around line 400-407: The current NimBLEScan::setScanResponseTimeout updates
m_srTimeoutTicks but leaves an already queued m_srTimer callout on its old
deadline; change the logic so when timeoutMs > 0 you convert to ticks
(ble_npl_time_ms_to_ticks) and then re-arm the head waiter: if m_srTimer is
active/queued, stop and restart/reset the callout with the new m_srTimeoutTicks
(use ble_npl_callout_stop and then ble_npl_callout_reset or the appropriate
callout init/reset API) so the pending callback uses the updated timeout; keep
the existing zero-path that stops the callout and clears m_srTimeoutTicks.
In `@src/NimBLEScan.h`:
- Around line 126-146: The toString() method currently preallocates out to 400
bytes and calls snprintf without adjusting the std::string size afterward;
capture snprintf's return value (the number of bytes written), check for
non-negative result, and resize out to that length (or to 0 on error) before
returning so the returned std::string reflects the actual bytes written (update
code in toString() where snprintf(...) is called and then call out.resize(...)
using the snprintf return value).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86e29232-dfa6-4bcc-9dcb-4d476dc073e1
📒 Files selected for processing (4)
src/NimBLEAdvertisedDevice.cppsrc/NimBLEAdvertisedDevice.hsrc/NimBLEScan.cppsrc/NimBLEScan.h
h2zero
force-pushed
the
scan-response-timer
branch
from
884207a to
176cc2d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/NimBLEScan.cpp (1)
137-173: Solid removal logic, though there be a minor race window, matey.Arr, the sentinel check on line 142 be outside the critical section. If another swab removes the device between yer check and enterin' the critical section, ye'd enter unnecessarily but the traversal would safely find nothin'. This be benign since it's just an optimization check, but ye could move it inside the critical section fer extra tightness if ye be feelin' cautious.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEScan.cpp` around lines 137 - 173, The sentinel check for "not in list" in NimBLEScan::removeWaitingDevice is performed before entering the critical section, creating a small race; to fix, move the pDev->m_pNextWaiting == pDev check inside the critical region protected by ble_npl_hw_enter_critical()/ble_npl_hw_exit_critical() (keep the null check outside if desired), so the presence test and list mutation happen atomically; ensure you still restore pDev->m_pNextWaiting = pDev and call resetWaitingTimer() as before after exiting the critical section.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NimBLEScan.cpp`:
- Around line 348-359: The clearWaitingList() call currently zeroes
pScan->m_pWaitingListHead before processing waiting devices, making the while
loop that calls pScan->m_stats.incMissedSrCount(),
pScan->removeWaitingDevice(pDev), sets pDev->m_callbackSent = 2 and calls
pScan->m_pScanCallbacks->onResult(pDev) dead code; move the logic that iterates
and processes the waiting list to run before pScan->clearWaitingList() so missed
SR stats and callbacks fire, and mirror the consistency in srTimerCb by adding
an erase call when m_maxResults == 0 if necessary.
---
Nitpick comments:
In `@src/NimBLEScan.cpp`:
- Around line 137-173: The sentinel check for "not in list" in
NimBLEScan::removeWaitingDevice is performed before entering the critical
section, creating a small race; to fix, move the pDev->m_pNextWaiting == pDev
check inside the critical region protected by
ble_npl_hw_enter_critical()/ble_npl_hw_exit_critical() (keep the null check
outside if desired), so the presence test and list mutation happen atomically;
ensure you still restore pDev->m_pNextWaiting = pDev and call
resetWaitingTimer() as before after exiting the critical section.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 76a1816c-b3a9-44c5-9dd4-934a9f03ba83
📒 Files selected for processing (4)
src/NimBLEAdvertisedDevice.cppsrc/NimBLEAdvertisedDevice.hsrc/NimBLEScan.cppsrc/NimBLEScan.h
🚧 Files skipped from review as they are similar to previous changes (3)
- src/NimBLEAdvertisedDevice.cpp
- src/NimBLEAdvertisedDevice.h
- src/NimBLEScan.h
h2zero
force-pushed
the
scan-response-timer
branch
2 times, most recently
from
b5f0399 to
2bac48d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/NimBLEScan.cpp (1)
142-147: Arr, this sentinel check be outside the critical section, unlike its sister inaddWaitingDevice.In
addWaitingDevice(line 114), the sentinel check be inside the critical section. Here, it be outside. While this likely won't cause real trouble on the high seas (the removal loop would just not find an already-removed device), 'twould be more consistent to move this check inside the critical section fer defensive codin'.🏴☠️ Suggested fix fer consistency
void NimBLEScan::removeWaitingDevice(NimBLEAdvertisedDevice* pDev) { if (pDev == nullptr) { return; } - if (pDev->m_pNextWaiting == pDev) { - return; // Not in the list - } - bool resetTimer = false; ble_npl_hw_enter_critical(); + + if (pDev->m_pNextWaiting == pDev) { + ble_npl_hw_exit_critical(0); + return; // Not in the list + } + if (m_pWaitingListHead == pDev) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEScan.cpp` around lines 142 - 147, Move the sentinel check (pDev->m_pNextWaiting == pDev) into the same critical section as in addWaitingDevice: call ble_npl_hw_enter_critical() first, then test the sentinel; if it indicates "not in list" ensure you ble_npl_hw_exit_critical() before returning. Update the block around the boolean resetTimer and the subsequent list-removal logic so the enter/exit critical pair encloses the sentinel check and the removal operations, matching the locking strategy used by addWaitingDevice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NimBLEAdvertisedDevice.h`:
- Around line 161-167: The default-constructed NimBLEAdvertisedDevice leaves
m_pNextWaiting as nullptr which is inconsistent with the sentinel convention
(other ctor sets m_pNextWaiting = this) and can break waiting-list checks; fix
by removing the public default constructor (make NimBLEAdvertisedDevice() =
delete or make it private) so instances must be created via the parametrized
constructor that sets m_pNextWaiting = this, or alternatively implement the
default constructor to initialize m_pNextWaiting = this; update the class
declaration around NimBLEAdvertisedDevice and its constructors accordingly.
---
Nitpick comments:
In `@src/NimBLEScan.cpp`:
- Around line 142-147: Move the sentinel check (pDev->m_pNextWaiting == pDev)
into the same critical section as in addWaitingDevice: call
ble_npl_hw_enter_critical() first, then test the sentinel; if it indicates "not
in list" ensure you ble_npl_hw_exit_critical() before returning. Update the
block around the boolean resetTimer and the subsequent list-removal logic so the
enter/exit critical pair encloses the sentinel check and the removal operations,
matching the locking strategy used by addWaitingDevice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73612f5b-981b-4059-a5b2-9a6d6b36dd3d
📒 Files selected for processing (4)
src/NimBLEAdvertisedDevice.cppsrc/NimBLEAdvertisedDevice.hsrc/NimBLEScan.cppsrc/NimBLEScan.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NimBLEScan.h
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/NimBLEScan.h (1)
126-146:⚠️ Potential issue | 🟡 MinorResize
getStatsString()to the bytes actually written.Arr,
out.resize(400)makes the returnedstd::stringreport length 400 even whensnprintf()writes far less. Callers of this new public API will get embedded NUL padding unless the string is shrunk towritten.🏴☠️ Suggested fix
std::string toString() const { std::string out; out.resize(400); // should be more than enough for the stats string - snprintf(&out[0], - out.size(), + int written = snprintf(&out[0], + out.size(), "Scan stats:\n" " Devices seen : %" PRIu32 "\n" " Duplicate advs : %" PRIu32 "\n" " Scan responses : %" PRIu32 "\n" " SR timing (ms) : min=%" PRIu32 ", max=%" PRIu32 ", avg=%" PRIu64 "\n" " Orphaned SR : %" PRIu32 "\n" " Missed SR : %" PRIu32 "\n", devCount, dupCount, srCount, srCount ? srMinMs : 0, srCount ? srMaxMs : 0, srCount ? srTotalMs / srCount : 0, orphanedSrCount, missedSrCount); + if (written < 0) { + out.clear(); + return out; + } + out.resize(static_cast<size_t>(written) < out.size() ? static_cast<size_t>(written) + : out.size() - 1); return out; }In C++, if `std::string out` is resized to 400 bytes and `snprintf(&out[0], out.size(), "...")` writes a shorter string, does `out.size()` remain 400 until `resize()` is called again?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEScan.h` around lines 126 - 146, The toString() implementation preallocates out to 400 bytes but never updates its size after snprintf(), so the returned std::string contains NUL padding; fix by capturing snprintf's return value (int written = snprintf(...)), then if written >= 0 and written < static_cast<int>(out.size()) call out.resize(static_cast<size_t>(written)) (or if written >= static_cast<int>(out.size()) optionally increase the buffer and retry/truncate accordingly) so the returned string length matches the actual bytes written from toString().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NimBLEScan.cpp`:
- Around line 622-625: The stop() implementation is prematurely calling
clearWaitingList() and clearResults(), which can drop pending scan waiters that
should be flushed by the BLE_GAP_EVENT_DISC_COMPLETE path (see
BLE_GAP_EVENT_DISC_COMPLETE and onResult() logic); modify stop() so it does not
tear down pending waiters/results directly — either remove the
clearWaitingList()/clearResults() calls from stop() and let the DISC_COMPLETE
handler perform cleanup, or explicitly flush the waiting list by invoking the
same onResult()/flush logic used in the BLE_GAP_EVENT_DISC_COMPLETE handler
before clearing; update references: stop(), clearWaitingList(), clearResults(),
BLE_GAP_EVENT_DISC_COMPLETE, and onResult() so pending scannable devices are
reported instead of dropped.
- Around line 332-337: The code currently only calls
pScan->addWaitingDevice(...) when pScan->m_srTimeoutTicks is nonzero, preventing
scannable legacy advertisers from being queued when setScanResponseTimeout(0) is
used; change the logic so that when isLegacyAdv &&
advertisedDevice->isScannable() you always call
pScan->addWaitingDevice(advertisedDevice) and then call
pScan->resetWaitingTimer() exactly as before (resetWaitingTimer already handles
m_srTimeoutTicks == 0), ensuring BLE_GAP_EVENT_DISC_COMPLETE can flush the
waiting list and onResult() will be invoked.
---
Duplicate comments:
In `@src/NimBLEScan.h`:
- Around line 126-146: The toString() implementation preallocates out to 400
bytes but never updates its size after snprintf(), so the returned std::string
contains NUL padding; fix by capturing snprintf's return value (int written =
snprintf(...)), then if written >= 0 and written < static_cast<int>(out.size())
call out.resize(static_cast<size_t>(written)) (or if written >=
static_cast<int>(out.size()) optionally increase the buffer and retry/truncate
accordingly) so the returned string length matches the actual bytes written from
toString().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10a7d503-8702-4c63-bd7e-80c8b33f2d94
📒 Files selected for processing (4)
src/NimBLEAdvertisedDevice.cppsrc/NimBLEAdvertisedDevice.hsrc/NimBLEScan.cppsrc/NimBLEScan.h
h2zero
force-pushed
the
scan-response-timer
branch
from
2bac48d to
4b3b912
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/NimBLEScan.cpp (1)
263-309: The device creation and update handling be well-integrated with the new stats and waiting list, captain!Good additions:
- Timestamping devices at creation (line 280) and on duplicates (line 295)
- Recording SR timing on scan response receipt (line 286)
- Removing from waiting list when SR arrives (line 289)
The duplicate handling at lines 297-300 re-adds scannable devices to the waiting list tail, maintaining FIFO timeout order. I notice this re-ordering is still gated by
m_srTimeoutTicks(line 297). Per a past review discussion, devices should still be queued even when the timeout is disabled, to allowBLE_GAP_EVENT_DISC_COMPLETEto flush them. Consider removing them_srTimeoutTicks &&guard here.♻️ Proposed fix to always re-queue scannable devices
// Restart scan-response timeout when we see a new non-scan-response // legacy advertisement during active scanning for a scannable device. advertisedDevice->m_time = ble_npl_time_get(); // Re-add to the tail so FIFO timeout order matches advertisement order. - if (pScan->m_srTimeoutTicks && advertisedDevice->isScannable()) { + if (advertisedDevice->isScannable()) { pScan->removeWaitingDevice(advertisedDevice); pScan->addWaitingDevice(advertisedDevice); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEScan.cpp` around lines 263 - 309, Duplicate handling currently only re-queues scannable devices when pScan->m_srTimeoutTicks is set; remove that guard so devices are always added to the waiting list to ensure BLE_GAP_EVENT_DISC_COMPLETE can flush them. Edit the block in NimBLEScan.cpp where advertisedDevice is handled (the else branch updating duplicates) and remove the "pScan->m_srTimeoutTicks &&" condition so that if advertisedDevice->isScannable() is true you always call pScan->removeWaitingDevice(advertisedDevice) and pScan->addWaitingDevice(advertisedDevice); keep the existing calls to removeWaitingDevice and addWaitingDevice and leave the rest of the duplicate logic (m_time update, m_callbackSent reset) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/NimBLEScan.cpp`:
- Around line 263-309: Duplicate handling currently only re-queues scannable
devices when pScan->m_srTimeoutTicks is set; remove that guard so devices are
always added to the waiting list to ensure BLE_GAP_EVENT_DISC_COMPLETE can flush
them. Edit the block in NimBLEScan.cpp where advertisedDevice is handled (the
else branch updating duplicates) and remove the "pScan->m_srTimeoutTicks &&"
condition so that if advertisedDevice->isScannable() is true you always call
pScan->removeWaitingDevice(advertisedDevice) and
pScan->addWaitingDevice(advertisedDevice); keep the existing calls to
removeWaitingDevice and addWaitingDevice and leave the rest of the duplicate
logic (m_time update, m_callbackSent reset) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09547ab9-acdf-4f8c-ae36-9ff3d02f7614
📒 Files selected for processing (4)
src/NimBLEAdvertisedDevice.cppsrc/NimBLEAdvertisedDevice.hsrc/NimBLEScan.cppsrc/NimBLEScan.h
✅ Files skipped from review due to trivial changes (1)
- src/NimBLEAdvertisedDevice.cpp
This ensures that the callback will be called within the configured time (in ms) when devices fail to respond to a scan response request within that time. * Adds stats when debug logging to help tune the scan response timeout/scan parameters.
h2zero
force-pushed
the
scan-response-timer
branch
from
4b3b912 to
31938ed
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This ensures that the callback will be called within the configured time (in ms) when devices fail to respond to a scan response request within that time.
Summary by CodeRabbit
New Features
Bug Fixes / Behavior